Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add PHPStan and ensure PHP 8.2 support #2792

Merged
merged 9 commits into from
Sep 21, 2023
Merged

Add PHPStan and ensure PHP 8.2 support #2792

merged 9 commits into from
Sep 21, 2023

Conversation

raviks789
Copy link
Collaborator

Dynamic properties are deprecated in PHP 8.2 and must be avoided.

@raviks789 raviks789 self-assigned this Aug 29, 2023
@cla-bot cla-bot bot added the cla/signed label Aug 29, 2023
nilmerg
nilmerg requested changes Aug 31, 2023
View reviewed changes
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add phpstan with level 2 to the workflow configuration. Plus a baseline configuration.

@nilmerg
Copy link
Member

nilmerg commented Aug 31, 2023

Workflow configuration is available in master since today. And of course, add PHP 8.2 to the matrix.

@raviks789 raviks789 force-pushed the fix/dynamic-property-deprecation branch from 793b763 to 49b6c3f Compare August 31, 2023 09:01
@raviks789 raviks789 force-pushed the fix/dynamic-property-deprecation branch 4 times, most recently from ad137cf to ebae558 Compare August 31, 2023 15:30
nilmerg
nilmerg reviewed Sep 1, 2023
View reviewed changes
.github/workflows/php.yml Outdated Show resolved Hide resolved
@nilmerg
Copy link
Member

nilmerg commented Sep 1, 2023

Please also take a look at the unit test output on PHP 8.1 and 8.2. There are still some deprecations logged.

@raviks789 raviks789 force-pushed the fix/dynamic-property-deprecation branch from ebae558 to 61c3c85 Compare September 1, 2023 08:08
@raviks789 raviks789 force-pushed the fix/dynamic-property-deprecation branch from 61c3c85 to b61c7be Compare September 1, 2023 08:24
@raviks789 raviks789 force-pushed the fix/dynamic-property-deprecation branch from 61f2294 to f319ebc Compare September 1, 2023 09:01
@raviks789
Copy link
Collaborator Author

raviks789 commented Sep 1, 2023 •
edited
Loading

Please also take a look at the unit test output on PHP 8.1 and 8.2. There are still some deprecations logged.

This is because the replaced ramsey/collection package does not have the PHP 8.1 support patch written in icinga-php-thirdparty. There is an issue gipfl/icinga-bundles#1 opened in gipfl/icinga-bundles repository for this.

@nilmerg nilmerg changed the title Avoid usage of dynamic properties Add PHPStan and ensure PHP 8.2 support Sep 1, 2023
Thomas-Gelf
Thomas-Gelf requested changes Sep 4, 2023
View reviewed changes
Copy link
Contributor

@Thomas-Gelf Thomas-Gelf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raviks789: good work, thank you! Could you please install the incubator version required in our module.info? If parsing the file annoys you, I'm fine with a hard-coded one for our tests, which can be raised once required.

Testing with unreleased dependencies is a good thing, which could be added as an optional/additional workflow. Main priority should be testing what's going to be installed. This then also removes the requirement to track incubator module dependencies in a second place.

Many thanks
Thomas

@nilmerg
Copy link
Member

nilmerg commented Sep 4, 2023

The installed incubator version has been introduced by #2309, not here. And this has the same intended effect as the just mentioned change: Testing the current state. If you want a stable test workflow, we should add this separately. Though, only for release/* branches or similar, there's no reason to run such a test on every push.

@Thomas-Gelf
Copy link
Contributor

For the Director testing with library versions it will never run with has absolutely no purpose. Here we're testing the current Director state, and not any intermediary foreign library state, that might or might not be released.

@antonfischl1980 antonfischl1980 mentioned this pull request Sep 11, 2023
Closed
@Thomas-Gelf Thomas-Gelf merged commit 228cbc8 into master Sep 21, 2023
12 checks passed
@Thomas-Gelf
Copy link
Contributor

Merged, thank you!

@Thomas-Gelf Thomas-Gelf deleted the fix/dynamic-property-deprecation branch September 21, 2023 07:01
@Thomas-Gelf Thomas-Gelf added this to the v1.11.0 milestone Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thomas-Gelf Thomas-Gelf Thomas-Gelf requested changes

@nilmerg nilmerg nilmerg approved these changes

Assignees

@raviks789 raviks789

Labels
cla/signed
Projects
None yet
Milestone
v1.11.0
Development

Successfully merging this pull request may close these issues.
3 participants
@raviks789 @nilmerg @Thomas-Gelf